Repository navigation
Update precommit hooks to ruff 0.16.10 - #246
Conversation
|
Started to review now. |
|
LGTM. It's hard to review every single change because of how many files are there, but I reviewed some of the key commits that seemed to be more than "just reformat" and it everything seems sensible, taking your PR description in mind. I'd be fine with merging this and testing on dev, which would give us some reassurance that these formatting PRs didn't break anything. |
|
Okay, here are some preliminary findings. I have the full paper-trail if you're interested in looking at it at some point (not very interesting, as you may imagine), but here's the summary:
I think resolving the last point before doing the full-month check for SWAPI/SWE is best. |
* messed up in c362dd1
|
Good catch. That was one of those cases of an IndexError from too few elements being caught by a broad |
|
GLOWS L3b+ have been validated, with the caveats that follow. L3b + L3c are identical. L3d has differences of order ~0.027% in 2 of 1068 I believe the differences can be explained by the fact that the SDC files used an older SPICE kernels. Pending any alarm bells that this might have raised for you, I would say GLOWS is fine. Still looking into SWAPI/SWE. |
|
Yes, those differences are tiny, and easily explained by updated SPICE kernels or external dependencies. Any problems from my changes are far more likely to be disastrous failure or huge changes. |
|
Okay, I just finished testing SWAPI & SWE and I think we're good to go. It took me a while because I was seeing some crazy differences in SWAPI's To conclude these are benign, I picked a file with large differences between the SDC and my reprocess, reverted my repo back to the exact same commit that generated the SDC file, processed it again, and confirmed that this file is bit-identical to the file generated by your branch. All of this to say: LGTM! |
cdeab29
into
IMAP-Science-Operations-Center:main
This is basically a sequel to #241 and #242 that does the same work for updating to ruff 0.16.10 (as suggested in #225). However I didn't do the black AST-checking here, assuming that the formatting is reasonably stable.
Some notes:
noqaas much as possible. However, that wasn't always possible, and I think we can just accept this as technical debt to potentially address later. In some cases I'm pretty sure the linter issues can only be fixed by a substantial refactoring, which would probably be good but would need careful coverage checks and such as well.DTZ) because we heavily use naive datetimes assuming UTC. Probably worth thinking about on a project level at some point.warnings.warn, but I wanted to preserve behavior out of caution for what might happen in our containerized environmentsubprocess.run. Basically if therunwas the end of a chain of logic, I usually added a check, figuring that blowing up spectacularly is okay if there was no "next step" that was going to happenexcept Exceptionthat made me very unhappy; some are in quite largetryblocks. The chance that we might have real bugs being squashed there is real. If a real fix wasn't obvious, I tried to focus a little bit where I could. In other cases I justnoqa'd it. This is real technical debt to address.Because of the last two items in particular, there is a very good chance that any code without robust unit tests might wind up failing when deployed to production. I think that's a chance worth taking. Maybe we should try running a bunch of files through in dev before merging?